UX improvement in relayer #541

adizere · 2021-01-19T18:06:03Z

Closes: #540
Closes: #536

Summary of changes:

For all tx raw commands, removed the unwrap call after ChainRuntime::spawn and added error-handling code. This results in JSON error output in case the light client is not initialized, instead of panics:

{"status":"error","result":["chain runtime/handle error: Light client supervisor error for chain id ibc-1: missing light client primary peer config"]}

For queries, the error returned if the target chain is not running is now more explicit, including the endpoint IP addr:

{"status":"error","result":["query error: RPC error to endpoint tcp://localhost:26657: error trying to connect: tcp connect error: Connection refused (os error 61) (code: 0)"]}

the start command now runs the v0 relayer; deleted the v0 command
introduced a log_level parameter in the relayer configuration file, so now we can enable error-only logging if we wish.
- a final output from the program will always exist in JSON format.
- introducing the prevaleent use of tracing info!, debug! etc macros, as replacements for abscissa status_... macros; both tracing and abscissa macros will adhere to the log_level parameter.

For contributor use:

Updated the Unreleased section of CHANGELOG.md with the issue.
If applicable: Unit tests written, added test to CI.
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Updated relevant documentation (docs/) and code comments.
Re-reviewed Files changed in the Github PR explorer.

codecov-io · 2021-01-20T09:01:15Z

Codecov Report

Merging #541 (d638cc3) into master (b1b37f5) will increase coverage by 26.7%.
The diff coverage is 69.4%.

@@            Coverage Diff            @@
##           master    #541      +/-   ##
=========================================
+ Coverage    13.6%   40.4%   +26.7%     
=========================================
  Files          69     134      +65     
  Lines        3752    8487    +4735     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    3434    +2921     
- Misses       2618    5053    +2435     
+ Partials      621       0     -621

Impacted Files	Coverage Δ
...application/ics20_fungible_token_transfer/error.rs	`0.0% <0.0%> (ø)`
...pplication/ics20_fungible_token_transfer/events.rs	`0.0% <ø> (ø)`
...ion/ics20_fungible_token_transfer/msgs/transfer.rs	`0.0% <0.0%> (ø)`
modules/src/events.rs	`0.0% <0.0%> (ø)`
modules/src/ics02_client/error.rs	`100.0% <ø> (ø)`
modules/src/ics02_client/events.rs	`0.0% <0.0%> (ø)`
modules/src/ics02_client/raw.rs	`0.0% <0.0%> (ø)`
modules/src/ics03_connection/error.rs	`100.0% <ø> (+66.6%)`	⬆️
modules/src/ics03_connection/events.rs	`0.0% <0.0%> (ø)`
modules/src/ics04_channel/error.rs	`100.0% <ø> (+75.0%)`	⬆️
... and 229 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a79d35...fc67361. Read the comment docs.

ancazamfir

Great improvements! Could you please also update the relayer operating instructions and the .toml example files to show how to control the logging.

adizere · 2021-01-20T18:26:10Z

Great improvements! Could you please also update the relayer operating instructions and the .toml example files to show how to control the logging.

Good point, that's something I overlooked!
d638cc3

romac

Looks great! Left a couple suggestions but feel free to ignore if you feel it doesn't improve things much or can just come at a later point.

romac · 2021-01-20T20:43:51Z

relayer-cli/src/commands/keys/add.rs

+            Ok(r) => Output::with_success().with_result(json!(r)).exit(),
+            Err(e) => Output::with_error()
+                .with_result(json!(format!("{}", e)))
+                .exit(),


I wonder if we could introduce a couple helpers to streamline such code a bit, eg.

Suggested change

Ok(r) => Output::with_success().with_result(json!(r)).exit(),

Err(e) => Output::with_error()

.with_result(json!(format!("{}", e)))

.exit(),

Ok(r) => Output::success(json!(r)).exit(),

Err(e) => Output::error(json!(format!("{}", e))).exit(),

Additionally, what do you think about changing the type of success/error/with_result to

pub fn with_result(mut self, res: impl Serialize) -> Self { self.result.push(serde_json::to_value(res).unwrap()); // same unwrap as in `json!` macro self }

which would result into

Suggested change

Ok(r) => Output::with_success().with_result(json!(r)).exit(),

Err(e) => Output::with_error()

.with_result(json!(format!("{}", e)))

.exit(),

Ok(r) => Output::success(r).exit(),

Err(e) => Output::error(format!("{}", e)).exit(),

Having to call unwrap on the result of serde_json::to_value is unfortunate, but that's actually what the json! macro does under the hood so perhaps it's okay?

Also note that this would still allow passing a custom JSON object with the json! macro:

Output::success(json!({ "hello": "world" })).exit()

Reason is that json! returns a Value whose Serialize instance produces the same Value when given to to_value.

ancazamfir

Looks good. I agree with Romain's comments, it would be nice to have that in this PR if it's a quick change. I will leave this up to you.

relayer-cli/src/conclude.rs

* Avoid unwraps in client txs. Better LightClient errors. * Avoided unwrap calls in tx raw commands * Tagged relayer RPC error with the culprit IP address. * Replaced start with v0 * Introduced logging levels. WIP: remove status_ calls * Updated the sample config file * Adapted keys commands to JSON output * Transition to tracing macros & JSON outputs * Changelog * Added operation instructions. * Simplified Output usage; h/t Romain's suggestions. * Better err message for client consensus query

adizere added 3 commits January 19, 2021 19:03

Avoid unwraps in client txs. Better LightClient errors.

cc589eb

Avoided unwrap calls in tx raw commands

d50522d

Tagged relayer RPC error with the culprit IP address.

855b5fa

adizere added 4 commits January 20, 2021 11:06

Replaced start with v0

6da84d6

Introduced logging levels. WIP: remove status_ calls

7534bd7

Updated the sample config file

f7764a1

FMT fix

d761502

adizere changed the title ~~UX improvement in relayer txs~~ UX improvement in relayer Jan 20, 2021

adizere added 3 commits January 20, 2021 15:17

Adapted keys commands to JSON output

e6730ef

Transition to tracing macros & JSON outputs

1145edb

Changelog

5ffe823

adizere marked this pull request as ready for review January 20, 2021 15:18

adizere requested review from ancazamfir and romac as code owners January 20, 2021 15:18

ancazamfir reviewed Jan 20, 2021

View reviewed changes

Added operation instructions.

d638cc3

romac approved these changes Jan 20, 2021

View reviewed changes

ancazamfir approved these changes Jan 21, 2021

View reviewed changes

Simplified Output usage; h/t Romain's suggestions.

fc67361

adizere commented Jan 21, 2021

View reviewed changes

relayer-cli/src/conclude.rs Show resolved Hide resolved

Better err message for client consensus query

fe5b437

adizere merged commit 80d222b into master Jan 21, 2021

adizere deleted the adi/523_ux branch January 21, 2021 10:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UX improvement in relayer #541

UX improvement in relayer #541

adizere commented Jan 19, 2021 •

edited

Loading

codecov-io commented Jan 20, 2021 •

edited

Loading

ancazamfir left a comment

adizere commented Jan 20, 2021

romac left a comment

romac Jan 20, 2021

romac Jan 20, 2021 •

edited

Loading

ancazamfir left a comment

UX improvement in relayer #541

UX improvement in relayer #541

Conversation

adizere commented Jan 19, 2021 • edited Loading

Summary of changes:

codecov-io commented Jan 20, 2021 • edited Loading

Codecov Report

ancazamfir left a comment

Choose a reason for hiding this comment

adizere commented Jan 20, 2021

romac left a comment

Choose a reason for hiding this comment

romac Jan 20, 2021

Choose a reason for hiding this comment

romac Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

ancazamfir left a comment

Choose a reason for hiding this comment

adizere commented Jan 19, 2021 •

edited

Loading

codecov-io commented Jan 20, 2021 •

edited

Loading

romac Jan 20, 2021 •

edited

Loading